Skip to content

feat: disable node-sensitive endpoints during shutdown#2030

Open
Dunsin-cyber wants to merge 3 commits intogetAlby:masterfrom
Dunsin-cyber:disable-nodeEndpoint-onShutdown
Open

feat: disable node-sensitive endpoints during shutdown#2030
Dunsin-cyber wants to merge 3 commits intogetAlby:masterfrom
Dunsin-cyber:disable-nodeEndpoint-onShutdown

Conversation

@Dunsin-cyber
Copy link
Contributor

@Dunsin-cyber Dunsin-cyber commented Feb 4, 2026

fixes #2018
Implemented a ShutdownMiddleware and updated the WailsRequestRouter to block node-related requests when the node is shutting down.

Summary by CodeRabbit

  • New Features

    • Graceful shutdown handling: most API and Wails requests are blocked with a "Node is shutting down" 503 while health, status, info and specific prefixed routes remain accessible.
    • Shutdown state exposed to higher layers so UI/service/Wails guards can query current shutdown status.
  • Tests

    • Added tests validating blocked and allowed endpoints during shutdown.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Adds a shutdown flag to the service, exposes IsShuttingDown via API and Wails, introduces ShutdownMiddleware used by HTTP/Wails to return 503 for most endpoints during shutdown while allowing health/safe routes and /api/alby/*, and updates tests and mocks to cover shutdown behavior.

Changes

Cohort / File(s) Summary
Core Shutdown State Management
service/models.go, service/service.go, service/stop.go
Adds an atomic shuttingDown field and IsShuttingDown() bool on service; sets the flag true before cancellation and resets after wait in stop logic.
API Layer Exposure
api/models.go, api/api.go
Adds IsShuttingDown() bool to API interface and implements func (a *api) IsShuttingDown() bool delegating to the service.
Middleware & HTTP Service
middleware/shutdown.go, http/http_service.go
Adds ShutdownNotifier and ShutdownMiddleware that whitelist safe routes and /api/alby/*; integrates middleware into HTTP service and switches to aliased echo middleware usage.
HTTP Testing
http/http_service_test.go
Adds mocks/imports and wires IsShuttingDown returns into tests; adds TestShutdown_BlockedEndpoint and TestShutdown_AllowedEndpoint and updates existing tests to assert shutdown behavior.
Wails Layer
wails/wails_app.go, wails/wails_handlers.go
Adds WailsApp.CheckShutdown() and gates non-safe Wails routes with the shutdown check while allowing safe routes and /api/alby/*.
Testing Utilities / Mocks
tests/mocks/Service.go
Adds MockService.IsShuttingDown() plus typed Expecter/Call helpers and Run/Return/RunAndReturn support; small mock adjustments.
Misc
api/..., wails/...
Minor import/formatting adjustments (e.g., adding fmt) to support new checks.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as HTTP/Wails Handler
    participant ShutdownMW as Shutdown Middleware
    participant Service as Service (IsShuttingDown)
    participant Endpoint as Endpoint Handler

    Client->>Handler: Request (e.g., /api/peers)
    Handler->>ShutdownMW: pass request
    ShutdownMW->>Service: IsShuttingDown()?
    Service-->>ShutdownMW: true
    ShutdownMW-->>Client: 503 Service Unavailable ("Node is shutting down")

    Client->>Handler: Request (e.g., /api/health)
    Handler->>ShutdownMW: pass request
    ShutdownMW->>Endpoint: allowed (safe route)
    Endpoint-->>Client: 200 OK
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I tapped the flag and gave a thump,
Safe hops still pass while others slump,
The node tucks in, the endpoints sigh,
Wait a moment—then awake and fly,
I nibble logs and hum a pump.

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: disable node-sensitive endpoints during shutdown' accurately and concisely describes the main change: blocking API requests to node-sensitive endpoints when shutdown is in progress.
Linked Issues check ✅ Passed The pull request fully addresses issue #2018 by implementing mechanisms to prevent API requests to node-sensitive endpoints during shutdown and avoid crashes from concurrent node operations.
Out of Scope Changes check ✅ Passed All code changes are directly related to implementing shutdown protection: adding shutdown state tracking, middleware guards, endpoint whitelisting, and comprehensive test coverage—no unrelated modifications present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@http/http_service_test.go`:
- Around line 396-535: Remove the redundant
e.Use(middleware.ShutdownMiddleware(mockSvc)) calls from
TestShutdown_BlockedEndpoint and TestShutdown_AllowedEndpoint since
RegisterSharedRoutes already wires ShutdownMiddleware; keep all other setup
unchanged. In TestShutdown_AllowedEndpoint change the final assertion to
explicitly expect http.StatusOK for the GET /api/node/status response (use the
existing rec2 and request), replacing the current assert.NotEqual that checks
against 503. Ensure references are to ShutdownMiddleware, RegisterSharedRoutes,
TestShutdown_BlockedEndpoint, and TestShutdown_AllowedEndpoint so the changes
are made in the correct tests.

In `@wails/wails_handlers.go`:
- Around line 27-42: The router currently checks raw route strings against
safeRoutes in WailsRequestRouter which fails when query strings are present;
normalize the incoming route before the safeRoutes lookup by extracting just the
path (e.g., strip anything after '?' or use URL parsing) so the existing
safeRoutes map and the strings.HasPrefix(route, "/api/alby/") check operate on
the path-only value, then use that normalized path in isSafe and subsequent
logic (references: WailsRequestRouter, safeRoutes, isSafe, CheckShutdown).
🧹 Nitpick comments (1)
http/http_service.go (1)

93-96: Ensure shutdown 503s still carry a Request ID.

ShutdownMiddleware short-circuits before RequestID, so shutdown responses won’t get a request ID (and logs may miss it). Consider registering RequestID before the shutdown guard. Please confirm the intended Echo middleware order per v4 docs.

Suggested reorder
- e.Use(middleware.ShutdownMiddleware(httpSvc.api))
-
- e.Use(echoMiddleware.Recover())
- e.Use(echoMiddleware.RequestID())
+ e.Use(echoMiddleware.RequestID())
+ e.Use(middleware.ShutdownMiddleware(httpSvc.api))
+
+ e.Use(echoMiddleware.Recover())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable node-related API endpoints when node is shutting down

1 participant

Comments